ECOTRACK-1: Green Route Advisor: pre-shipment scenario comparison - #21
Open
Ostaps wants to merge 1 commit into
Open
ECOTRACK-1: Green Route Advisor: pre-shipment scenario comparison#21Ostaps wants to merge 1 commit into
Ostaps wants to merge 1 commit into
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR introduces Green Route Advisor v1, enabling users to compare two shipment scenarios side-by-side and automatically identify the one with lower estimated greenhouse gas emissions. The backend service processes scenario inputs through the existing emissions calculator and returns per-scenario estimates plus a preferred choice. The ShipmentHub UI adds scenario input controls, a comparison trigger, and results display, along with a status filter for shipments with auto-reset behavior. ChangesGreen Route Advisor v1: Scenario Comparison Feature
Sequence Diagram(s)sequenceDiagram
participant User
participant ShipmentHub
participant ShipmentAPI
participant ShipmentController
participant ShipmentService
participant SustainabilityService
User->>ShipmentHub: Enter Scenario A and B
User->>ShipmentHub: Click Compare Scenarios
ShipmentHub->>ShipmentAPI: compareShipmentScenarios(scenarios)
ShipmentAPI->>ShipmentController: POST /api/v1/shipments/compare
ShipmentController->>ShipmentService: compareScenarios(List)
ShipmentService->>SustainabilityService: calculateEmissions(Scenario A)
SustainabilityService-->>ShipmentService: estimatedCo2_A
ShipmentService->>SustainabilityService: calculateEmissions(Scenario B)
SustainabilityService-->>ShipmentService: estimatedCo2_B
ShipmentService->>ShipmentService: rank by minimum CO2
ShipmentService-->>ShipmentController: ScenarioComparisonResponseDTO
ShipmentController-->>ShipmentAPI: JSON response
ShipmentAPI-->>ShipmentHub: comparison results
ShipmentHub->>User: display scenarios with preferred flagged
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/main/java/com/ecotrack/service/ShipmentService.java`:
- Around line 113-115: The stream mapping over scenarios can encounter null
elements and cause a NullPointerException inside buildScenarioResult; before
mapping, filter out or validate null entries from the scenarios collection
(e.g., replace scenarios.stream().map(this::buildScenarioResult) with
scenarios.stream().filter(Objects::nonNull).map(this::buildScenarioResult) or
perform an explicit pre-check that throws a clear validation exception when any
scenario is null), or alternatively detect nulls and throw a domain validation
exception with a descriptive message so callers receive a proper validation
response instead of an NPE.
- Line 121: The current lambda in results.forEach uses
Objects.equals(result.getScenario(), preferred.getScenario()) which can mark
multiple items preferred if scenario names duplicate; change the comparison to
use object identity or a stable unique identifier instead — for example, in the
results.forEach(...) that calls result.setPreferred(...), compare result ==
preferred (reference equality) or compare a unique id getter (e.g.,
result.getId().equals(preferred.getId())) rather than comparing
result.getScenario() and preferred.getScenario().
- Around line 145-147: The catch block in ShipmentService that handles transport
mode parsing currently throws a new IllegalArgumentException without preserving
the original exception; change the throw to include the caught exception as the
cause (use the constructor that accepts a Throwable) so the original exception
`ex` is passed through when rethrowing from the catch in the method that parses
`input.getTransportMode()`.
In `@CHANGELOG.md`:
- Around line 9-10: Changelog headings like "### Added", "### Changed", and "###
Known Limitations" are missing a blank line after them (MD022); update
CHANGELOG.md to ensure each of those headings is followed by a single blank line
(e.g., add a newline after the "### Added" before the list item), and apply the
same fix for the other occurrences of those headings noted in the comment so
every section has a blank line after its heading.
In `@docs/green-route-advisor-v1.md`:
- Around line 3-4: Several Markdown section headings (e.g., "## Scope" and the
other headings called out in the review) are followed immediately by content
which violates markdownlint MD022; fix by inserting a single blank line
immediately after each affected heading (every line that begins with # or ## in
this document), ensuring headings such as "## Scope" have one empty line before
the following paragraph so the linter passes.
In `@frontend/src/pages/ShipmentHub.jsx`:
- Around line 226-227: The JSX uses compareResult.scenarios.map(...) with
key={scenario.scenario}, which may not be unique; update the map key to use a
stable unique identifier (preferably scenario.id or scenario.uuid) in the
scenario result card component instead of scenario.scenario, e.g.,
key={scenario.id}; if the scenario objects lack a unique id, add one upstream or
as a last resort use a deterministic composite key (e.g.,
`${scenario.scenario}-${scenario.timestamp || idx}`) inside the
compareResult.scenarios.map callback to avoid relying on the array index alone.
- Around line 143-158: The current client-side check (hasInvalidScenario) only
tests presence so zeros/negatives slip through; update the validation to parse
distanceKm and payloadTons (use parseFloat) and ensure both are numbers > 0 (and
not NaN) for each scenario in scenarioForm, and also ensure vehicleId and
transportMode/origin/destination remain non-empty; if any parsed value is
invalid, show the toast and return before setCompareLoading(true). Adjust the
scenarios mapping (where scenarios is created) to rely on already-validated
parsed values so server-side rejections are avoided.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 99c579f7-cbdd-4a7e-8a59-cad209e4de5b
📒 Files selected for processing (10)
CHANGELOG.mdbackend/src/main/java/com/ecotrack/controller/ShipmentController.javabackend/src/main/java/com/ecotrack/dto/ScenarioComparisonRequestDTO.javabackend/src/main/java/com/ecotrack/dto/ScenarioComparisonResponseDTO.javabackend/src/main/java/com/ecotrack/dto/ScenarioInputDTO.javabackend/src/main/java/com/ecotrack/dto/ScenarioResultDTO.javabackend/src/main/java/com/ecotrack/service/ShipmentService.javadocs/green-route-advisor-v1.mdfrontend/src/api/shipments.jsfrontend/src/pages/ShipmentHub.jsx
Comment on lines
+113
to
+115
| List<ScenarioResultDTO> results = scenarios.stream() | ||
| .map(this::buildScenarioResult) | ||
| .collect(Collectors.toList()); |
There was a problem hiding this comment.
Guard against null scenario items before mapping.
If any element in scenarios is null, Line 114 triggers a NullPointerException in buildScenarioResult, causing an internal error instead of a clear validation response.
Suggested fix
List<ScenarioResultDTO> results = scenarios.stream()
+ .peek(s -> {
+ if (s == null) {
+ throw new IllegalArgumentException("Scenario entries must not be null");
+ }
+ })
.map(this::buildScenarioResult)
.collect(Collectors.toList());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/main/java/com/ecotrack/service/ShipmentService.java` around lines
113 - 115, The stream mapping over scenarios can encounter null elements and
cause a NullPointerException inside buildScenarioResult; before mapping, filter
out or validate null entries from the scenarios collection (e.g., replace
scenarios.stream().map(this::buildScenarioResult) with
scenarios.stream().filter(Objects::nonNull).map(this::buildScenarioResult) or
perform an explicit pre-check that throws a clear validation exception when any
scenario is null), or alternatively detect nulls and throw a domain validation
exception with a descriptive message so callers receive a proper validation
response instead of an NPE.
| .min(Comparator.comparing(ScenarioResultDTO::getEstimatedCo2)) | ||
| .orElseThrow(() -> new IllegalArgumentException("No scenarios to compare")); | ||
|
|
||
| results.forEach(result -> result.setPreferred(Objects.equals(result.getScenario(), preferred.getScenario()))); |
There was a problem hiding this comment.
Preferred flag computation is incorrect when scenario names are duplicated.
Line 121 compares by scenario name, so duplicate names can mark multiple cards as preferred. Mark preference by object identity (or index), not display name.
Suggested fix
- results.forEach(result -> result.setPreferred(Objects.equals(result.getScenario(), preferred.getScenario())));
+ results.forEach(result -> result.setPreferred(result == preferred));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/main/java/com/ecotrack/service/ShipmentService.java` at line 121,
The current lambda in results.forEach uses Objects.equals(result.getScenario(),
preferred.getScenario()) which can mark multiple items preferred if scenario
names duplicate; change the comparison to use object identity or a stable unique
identifier instead — for example, in the results.forEach(...) that calls
result.setPreferred(...), compare result == preferred (reference equality) or
compare a unique id getter (e.g., result.getId().equals(preferred.getId()))
rather than comparing result.getScenario() and preferred.getScenario().
Comment on lines
+145
to
+147
| } catch (Exception ex) { | ||
| throw new IllegalArgumentException("Unsupported transportMode: " + input.getTransportMode()); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "ShipmentService.java" -type fRepository: Ostaps/ecotrack
Length of output: 125
🏁 Script executed:
wc -l ./backend/src/main/java/com/ecotrack/service/ShipmentService.javaRepository: Ostaps/ecotrack
Length of output: 129
🏁 Script executed:
sed -n '135,155p' ./backend/src/main/java/com/ecotrack/service/ShipmentService.javaRepository: Ostaps/ecotrack
Length of output: 1015
🏁 Script executed:
cat > /tmp/verify_exception.java << 'EOF'
// Verifying IllegalArgumentException constructor
public class TestException {
public static void main(String[] args) {
try {
throw new Exception("root cause");
} catch (Exception ex) {
// This should compile - IllegalArgumentException accepts Throwable cause
throw new IllegalArgumentException("Message", ex);
}
}
}
EOF
# Just checking Java syntax - IllegalArgumentException(String message, Throwable cause) is standard Java API
echo "IllegalArgumentException constructors:"
echo "1. IllegalArgumentException(String message)"
echo "2. IllegalArgumentException(String message, Throwable cause)"
echo "3. IllegalArgumentException(Throwable cause)"Repository: Ostaps/ecotrack
Length of output: 248
Preserve the original exception cause when wrapping transport mode parse errors.
Line 146 drops the caught exception ex, making error diagnostics and debugging harder. Pass the exception as the cause parameter to maintain the full error chain.
Suggested fix
} catch (Exception ex) {
- throw new IllegalArgumentException("Unsupported transportMode: " + input.getTransportMode());
+ throw new IllegalArgumentException("Unsupported transportMode: " + input.getTransportMode(), ex);
}🧰 Tools
🪛 PMD (7.24.0)
[Medium] 146-146: PreserveStackTrace (Best Practices): Thrown exception does not preserve the stack trace of exception 'ex' on all code paths
(PreserveStackTrace (Best Practices))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/main/java/com/ecotrack/service/ShipmentService.java` around lines
145 - 147, The catch block in ShipmentService that handles transport mode
parsing currently throws a new IllegalArgumentException without preserving the
original exception; change the throw to include the caught exception as the
cause (use the constructor that accepts a Throwable) so the original exception
`ex` is passed through when rethrowing from the catch in the method that parses
`input.getTransportMode()`.
Comment on lines
+9
to
+10
| ### Added | ||
| - Green Route Advisor v1 scenario comparison endpoint: `POST /api/v1/shipments/compare`. |
There was a problem hiding this comment.
Fix heading spacing in changelog sections (MD022).
### Added, ### Changed, and ### Known Limitations should each be followed by a blank line.
Proposed fix
### Added
+
- Green Route Advisor v1 scenario comparison endpoint: `POST /api/v1/shipments/compare`.
@@
### Changed
+
- Shipment service now compares at least two scenarios using existing `SustainabilityService` logic, with explicit rule `MIN_ESTIMATED_CO2E` and methodology version `GLEC Framework v3`.
@@
### Known Limitations
+
- Full project frontend lint remains blocked by pre-existing issues outside feature scope.Also applies to: 19-20, 23-24
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 9-9: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CHANGELOG.md` around lines 9 - 10, Changelog headings like "### Added", "###
Changed", and "### Known Limitations" are missing a blank line after them
(MD022); update CHANGELOG.md to ensure each of those headings is followed by a
single blank line (e.g., add a newline after the "### Added" before the list
item), and apply the same fix for the other occurrences of those headings noted
in the comment so every section has a blank line after its heading.
Comment on lines
+3
to
+4
| ## Scope | ||
| Green Route Advisor v1 adds pre-shipment scenario comparison in Shipment Hub while reusing the existing emissions calculation path. |
There was a problem hiding this comment.
Add blank lines after section headings to satisfy markdownlint MD022.
Several headings are followed immediately by content. Insert one empty line after each affected heading so docs lint passes consistently.
Proposed fix
## Scope
+
Green Route Advisor v1 adds pre-shipment scenario comparison in Shipment Hub while reusing the existing emissions calculation path.
@@
## API Contract
+
Endpoint: `POST /api/v1/shipments/compare`
@@
## UI Behavior (Shipment Hub)
+
File: `frontend/src/pages/ShipmentHub.jsx`
@@
## Related Shipment Hub Fix
+
The Shipment Hub status dropdown now:
@@
## Verification Status
+
Implemented and wired across backend/frontend. Full-suite validation is partially blocked by known pre-existing/global environment issues:Also applies to: 16-17, 35-36, 43-44, 50-51
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 3-3: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/green-route-advisor-v1.md` around lines 3 - 4, Several Markdown section
headings (e.g., "## Scope" and the other headings called out in the review) are
followed immediately by content which violates markdownlint MD022; fix by
inserting a single blank line immediately after each affected heading (every
line that begins with # or ## in this document), ensuring headings such as "##
Scope" have one empty line before the following paragraph so the linter passes.
Comment on lines
+143
to
+158
| const hasInvalidScenario = scenarioForm.some( | ||
| s => !s.origin || !s.destination || !s.distanceKm || !s.payloadTons || !s.transportMode || !s.vehicleId | ||
| ); | ||
| if (hasInvalidScenario) { | ||
| toast.error('Fill all fields for both scenarios.'); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| setCompareLoading(true); | ||
| const scenarios = scenarioForm.map(s => ({ | ||
| ...s, | ||
| distanceKm: parseFloat(s.distanceKm), | ||
| payloadTons: parseFloat(s.payloadTons), | ||
| vehicleId: s.vehicleId | ||
| })); |
There was a problem hiding this comment.
Client-side validation should enforce positive numeric inputs before compare.
Current checks only verify presence, so 0/negative values pass and then fail server-side. Validate parsed numbers as > 0 to prevent avoidable round-trips.
Suggested fix
const hasInvalidScenario = scenarioForm.some(
- s => !s.origin || !s.destination || !s.distanceKm || !s.payloadTons || !s.transportMode || !s.vehicleId
+ s => {
+ const distance = Number(s.distanceKm);
+ const payload = Number(s.payloadTons);
+ return (
+ !s.origin ||
+ !s.destination ||
+ !s.transportMode ||
+ !s.vehicleId ||
+ !Number.isFinite(distance) ||
+ !Number.isFinite(payload) ||
+ distance <= 0 ||
+ payload <= 0
+ );
+ }
);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/pages/ShipmentHub.jsx` around lines 143 - 158, The current
client-side check (hasInvalidScenario) only tests presence so zeros/negatives
slip through; update the validation to parse distanceKm and payloadTons (use
parseFloat) and ensure both are numbers > 0 (and not NaN) for each scenario in
scenarioForm, and also ensure vehicleId and transportMode/origin/destination
remain non-empty; if any parsed value is invalid, show the toast and return
before setCompareLoading(true). Adjust the scenarios mapping (where scenarios is
created) to rely on already-validated parsed values so server-side rejections
are avoided.
Comment on lines
+226
to
+227
| {compareResult.scenarios.map((scenario) => ( | ||
| <div key={scenario.scenario} className={`rounded-lg border p-3 ${scenario.preferred ? 'border-green-400 bg-green-50' : 'border-gray-200 bg-white'}`}> |
There was a problem hiding this comment.
Use a stable unique key for scenario result cards.
key={scenario.scenario} can collide for duplicate names and cause React reconciliation glitches.
Suggested fix
- {compareResult.scenarios.map((scenario) => (
- <div key={scenario.scenario} className={`rounded-lg border p-3 ${scenario.preferred ? 'border-green-400 bg-green-50' : 'border-gray-200 bg-white'}`}>
+ {compareResult.scenarios.map((scenario, idx) => (
+ <div key={`${scenario.scenario ?? 'scenario'}-${idx}`} className={`rounded-lg border p-3 ${scenario.preferred ? 'border-green-400 bg-green-50' : 'border-gray-200 bg-white'}`}>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {compareResult.scenarios.map((scenario) => ( | |
| <div key={scenario.scenario} className={`rounded-lg border p-3 ${scenario.preferred ? 'border-green-400 bg-green-50' : 'border-gray-200 bg-white'}`}> | |
| {compareResult.scenarios.map((scenario, idx) => ( | |
| <div key={`${scenario.scenario ?? 'scenario'}-${idx}`} className={`rounded-lg border p-3 ${scenario.preferred ? 'border-green-400 bg-green-50' : 'border-gray-200 bg-white'}`}> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/pages/ShipmentHub.jsx` around lines 226 - 227, The JSX uses
compareResult.scenarios.map(...) with key={scenario.scenario}, which may not be
unique; update the map key to use a stable unique identifier (preferably
scenario.id or scenario.uuid) in the scenario result card component instead of
scenario.scenario, e.g., key={scenario.id}; if the scenario objects lack a
unique id, add one upstream or as a last resort use a deterministic composite
key (e.g., `${scenario.scenario}-${scenario.timestamp || idx}`) inside the
compareResult.scenarios.map callback to avoid relying on the array index alone.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implemented Green Route Advisor v1 compare flow across backend and shipment hub UI.
POST /api/v1/shipments/comparein shipment controller.ShipmentServiceusing existingSustainabilityServiceonly (no parallel engine).preferredScenario,rankingRule,methodologyVersion, and per-scenarioEstimatedvalues.All Statusesafter shipment creation).Task
Source (task / board)
Iterix · feature-dev · delivery feature ·
wf-feature-dev-20260520-084616Full task prompt and artifacts live under
.softi/workflows/wf-feature-dev-20260520-084616/.